ref: json serialization error reporting by armcknight · Pull Request #2355 · getsentry/sentry-cocoa

armcknight · 2022-11-03T19:17:25Z

I had a JSON validation error, but couldn't tell what it was since we simply check for the boolean from isValidJSON. I had to implement just attempting the serialization and inspecting the inout error it gives, and learned that at least in my case, serialization also throws exceptions in certain cases, but the exception told me exactly what the problem was. I figured it would be helpful to keep around, but also that we may not want the exception handler in production, so I used some conditional compilation to keep it behaving as it was in release configs, but in all other configs, to get the info from the exception/error.

To that end, added some new functions to construct NSErrors that can include an underlying NSError we may get from a Cocoa library method, or an NSException, with some light refactoring of the implementations in SentryError.

#skip-changelog

Just copying here some details from one of my commits for visibility:

before:
    - test if input is valid JSON with `-[NSJSONSerialization
    isValidJSON]` and if not, build our own error.
    - problem: no hints as to why the JSON is invalid

after:
    - attempt direct serialization with `-[NSJSONSerialization
    dataWithJSONObject:options:error:]` and use the library error
    - this can throw an exception in certain failure modes: surround
    the call with try/catch
    - use new functions in SentryError to create an NSError from
    either an NSException or the inout NSError
    - only compile like this for nonrelease builds so we don't incur
    the performance hit of throwing an exception (see apple's docs:
    "For best performance in 64-bit, you should throw exceptions only
    when absolutely necessary.")

before: - test if input is valid JSON with `-[NSJSONSerialization isValidJSON]` and if not, build our own error. - problem: no hints as to why the JSON is invalid after: - attempt direct serialization with `-[NSJSONSerialization dataWithJSONObject:options:error:]` and use the library error - this can throw an exception in certain failure modes: surround the call with try/catch - use new functions in SentryError to create an NSError from either an NSException or the inout NSError - only compile like this for nonrelease builds so we don't incur the performance hit of throwing an exception (see apple's docs: "For best performance in 64-bit, you should throw exceptions only when absolutely necessary.")

github-actions · 2022-11-03T19:32:25Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1229.96 ms	1257.78 ms	27.82 ms
Size	20.76 KiB	368.09 KiB	347.33 KiB

Baseline results on branch: master

Startup times

Revision	Plain	With Sentry	Diff
`347a8e9`	1243.50 ms	1265.90 ms	22.40 ms
`8b040e4`	1234.76 ms	1244.71 ms	9.95 ms
`bdfccaa`	1202.83 ms	1228.96 ms	26.13 ms
`075911d`	1256.73 ms	1271.22 ms	14.49 ms
`5025d2e`	1248.52 ms	1251.72 ms	3.20 ms
`4a0c282`	1228.60 ms	1250.74 ms	22.14 ms
`d73ebd0`	1200.83 ms	1236.10 ms	35.27 ms
`c2a9b60`	1222.10 ms	1240.62 ms	18.52 ms
`5025d2e`	1245.14 ms	1268.58 ms	23.44 ms
`6177f2d`	1206.55 ms	1226.20 ms	19.65 ms

App size

Revision	Plain	With Sentry	Diff
`347a8e9`	20.51 KiB	333.16 KiB	312.65 KiB
`8b040e4`	20.50 KiB	333.54 KiB	313.04 KiB
`bdfccaa`	20.50 KiB	361.80 KiB	341.29 KiB
`075911d`	20.51 KiB	332.90 KiB	312.39 KiB
`5025d2e`	20.51 KiB	331.79 KiB	311.28 KiB
`4a0c282`	20.51 KiB	333.10 KiB	312.60 KiB
`d73ebd0`	20.50 KiB	365.18 KiB	344.68 KiB
`c2a9b60`	20.50 KiB	333.54 KiB	313.04 KiB
`5025d2e`	20.51 KiB	331.79 KiB	311.28 KiB
`6177f2d`	20.51 KiB	332.90 KiB	312.40 KiB

Previous results on branch: armcknight/ref/json-serialization-error-reporting

Startup times

Revision	Plain	With Sentry	Diff
`7083f18`	1247.37 ms	1280.32 ms	32.95 ms
`fcf17d7`	1245.12 ms	1254.00 ms	8.88 ms
`cfa9b01`	1209.55 ms	1235.36 ms	25.81 ms

App size

Revision	Plain	With Sentry	Diff
`7083f18`	20.50 KiB	365.44 KiB	344.94 KiB
`fcf17d7`	20.50 KiB	365.44 KiB	344.94 KiB
`cfa9b01`	20.76 KiB	367.28 KiB	346.53 KiB

brustolin

LGTM

philipphofmann

Please add some tests for the new functionality.

armcknight · 2022-11-08T20:47:16Z

Added some basic tests for the new functions and a test for the serializer with the actual scenario I encountered, trying to add a NSDate directly to a JSON dict for serialization.

philipphofmann

Thanks for adding tests

armcknight · 2022-11-09T16:51:59Z

No idea why the tests are failing with a compiler error that doesn't make sense and doesn't happen locally; rerunning ⏳

armcknight · 2022-11-09T21:58:41Z

Even if I make the suggested changes locally, then my local build fails. Any idea what could be going on with this @philipphofmann?

armcknight · 2022-11-14T23:12:30Z

@brustolin thanks for pushing that commit, that one builds locally and on CI 👍🏻

…ents * master: build(deps): bump github/codeql-action from 2.1.31 to 2.1.32 (#2386) build(deps): bump fastlane from 2.210.1 to 2.211.0 (#2385) ref: json serialization error reporting (#2355) release: 7.31.0 ref: Fix outdated comment in SessionTracker (#2381) fix: Do not delete the app state (#2382) build: Split Swift and Clang format for pre-commit (#2380)

* master: (56 commits) meta: disable swiftlint file length check in TBDBClient.swift (#2435) Revert "test: shorten some tests (#2422)" (#2427) test: shorten some tests (#2422) test: include Sentry changes in hash keys (#2412) release: 7.31.2 fix: Crash in Client when reading integrations (#2398) test: tooling improvements (#2400) fix: Don't increase session's error count for dropped events (#2374) Update CHANGELOG.md (#2396) release: 7.31.1 Fix: Set the correct OOM event timestamp (#2394) test: Fix SentrySerializationTests.testSerializationFailsWithInvalidJSONObject (#2392) feat(hybrid-sdks): Add captureScreenshots to PrivateSDKOnly (#2384) ci: Increase test timeout for iOS 12 (#2391) test: Disable flaky testCrashReportCount1 (#2389) test: Disable flaky testSerializeWithUnderlyingNSException (#2387) build(deps): bump github/codeql-action from 2.1.31 to 2.1.32 (#2386) build(deps): bump fastlane from 2.210.1 to 2.211.0 (#2385) ref: json serialization error reporting (#2355) release: 7.31.0 ...

armcknight added 3 commits November 2, 2022 16:06

feat: build errors with exceptions and underling errors

3fe877a

refactor to extract common init

b959c35

armcknight requested review from brustolin and philipphofmann as code owners November 3, 2022 19:17

check for nil inout error in exception handler

dd01a3e

brustolin approved these changes Nov 4, 2022

View reviewed changes

kevinrenskers approved these changes Nov 7, 2022

View reviewed changes

philipphofmann reviewed Nov 7, 2022

View reviewed changes

add tests

a9e0435

philipphofmann approved these changes Nov 9, 2022

View reviewed changes

Update SentryNSErrorTests.swift

d9bcb31

armcknight merged commit 90ed869 into master Nov 14, 2022

armcknight deleted the armcknight/ref/json-serialization-error-reporting branch November 14, 2022 23:15

armcknight mentioned this pull request Jan 19, 2023

Misses a branch for a PR I've merged seachicken/gh-poi#94

Closed

philipphofmann mentioned this pull request Apr 4, 2023

fix: Serialization with invalid objects #2858

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

ref: json serialization error reporting#2355

ref: json serialization error reporting#2355
armcknight merged 6 commits intomasterfrom
armcknight/ref/json-serialization-error-reporting

armcknight commented Nov 3, 2022 •

edited

Loading

Uh oh!

github-actions bot commented Nov 3, 2022 •

edited

Loading

Baseline results on branch: master

Startup times

App size

Previous results on branch: armcknight/ref/json-serialization-error-reporting

Startup times

App size

Uh oh!

brustolin left a comment

Uh oh!

philipphofmann left a comment

Uh oh!

armcknight commented Nov 8, 2022

Uh oh!

philipphofmann left a comment

Uh oh!

armcknight commented Nov 9, 2022

Uh oh!

armcknight commented Nov 9, 2022

Uh oh!

armcknight commented Nov 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

armcknight commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Performance metrics 🚀

Baseline results on branch: master

Startup times

App size

Previous results on branch: armcknight/ref/json-serialization-error-reporting

Startup times

App size

Uh oh!

brustolin left a comment

Choose a reason for hiding this comment

Uh oh!

philipphofmann left a comment

Choose a reason for hiding this comment

Uh oh!

armcknight commented Nov 8, 2022

Uh oh!

philipphofmann left a comment

Choose a reason for hiding this comment

Uh oh!

armcknight commented Nov 9, 2022

Uh oh!

armcknight commented Nov 9, 2022

Uh oh!

armcknight commented Nov 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

armcknight commented Nov 3, 2022 •

edited

Loading

github-actions bot commented Nov 3, 2022 •

edited

Loading